Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Session replay performance for SwiftUI #4419

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

brustolin
Copy link
Contributor

📜 Description

Improved Performance for SwiftUI session replay

💚 How did you test it?

Samples

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

@brustolin brustolin changed the base branch from main to feat/SR-Start-Stop October 9, 2024 13:13
Copy link

github-actions bot commented Oct 9, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.76 ms 1262.71 ms 19.96 ms
Size 21.58 KiB 705.96 KiB 684.38 KiB

Baseline results on branch: feat/SR-Start-Stop

Startup times

Revision Plain With Sentry Diff
1529984 1229.36 ms 1247.45 ms 18.09 ms
814655e 1230.51 ms 1243.13 ms 12.61 ms

App size

Revision Plain With Sentry Diff
1529984 21.58 KiB 730.64 KiB 709.06 KiB
814655e 21.58 KiB 705.77 KiB 684.19 KiB

Previous results on branch: test/SR-Performance

Startup times

Revision Plain With Sentry Diff
0e4d5e9 1210.56 ms 1224.63 ms 14.07 ms
e2d4ed4 1237.67 ms 1246.39 ms 8.72 ms
13f95b7 1232.81 ms 1251.57 ms 18.76 ms

App size

Revision Plain With Sentry Diff
0e4d5e9 21.58 KiB 706.83 KiB 685.25 KiB
e2d4ed4 21.58 KiB 706.83 KiB 685.25 KiB
13f95b7 21.58 KiB 706.39 KiB 684.81 KiB

@@ -39,6 +39,10 @@ struct RedactRegion {
self.type = type
self.color = color
}

static func == (lhs: RedactRegion, rhs: RedactRegion) -> Bool {
lhs.size == rhs.size && lhs.transform == rhs.transform && lhs.type == rhs.type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: I guess you added this so you can compare the regions easily here:

guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue }

I'm not a fan of this cause the equals here doesn't include the color. This can be very confusing. I would prefer adding a helper method or something different if you only want to check if to redact regions are the same without considering the color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can do that.

for region in redact {
let rect = CGRect(origin: CGPoint.zero, size: region.size)
var transform = region.transform
let path = CGPath(rect: rect, transform: &transform)

defer { latestRegion = region }

guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: It seems like this speeds up the performance significantly. Maybe it's worth adding a comment for this, cause we don't have a test to validate that we do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a test to validate this.
Its the only test added to this PR

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.301%. Comparing base (40e71d9) to head (1ca9f1c).
Report is 1 commits behind head on feat/SR-Start-Stop.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                    @@
##           feat/SR-Start-Stop     #4419       +/-   ##
========================================================
+ Coverage              90.104%   91.301%   +1.196%     
========================================================
  Files                     607       610        +3     
  Lines                   49378     49696      +318     
  Branches                17443     17903      +460     
========================================================
+ Hits                    44492     45373      +881     
+ Misses                   4793      4231      -562     
+ Partials                   93        92        -1     
Files with missing lines Coverage Δ
Sources/Sentry/SentrySessionReplayIntegration.m 85.375% <100.000%> (+81.027%) ⬆️
Sources/Swift/Tools/SentryViewPhotographer.swift 92.592% <100.000%> (+18.592%) ⬆️
Sources/Swift/Tools/UIRedactBuilder.swift 92.537% <100.000%> (+1.628%) ⬆️
...ests/SentryTests/SentryViewPhotographerTests.swift 99.285% <100.000%> (+0.054%) ⬆️

... and 100 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40e71d9...1ca9f1c. Read the comment docs.

@brustolin brustolin merged commit 6a7ed71 into feat/SR-Start-Stop Oct 10, 2024
63 checks passed
@brustolin brustolin deleted the test/SR-Performance branch October 10, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants